added support for maximum expiration of a JWT token #3331

mvanholsteijn · 2018-03-22T16:01:29Z

Summary

Support for limiting the expiration period on JWT tokens.

In the JWT plugin you can set the property 'maximum_expiration' to a positive integer, indicating the maximum number of seconds the 'exp' claim in the token may be ahead in the future.

The purpose of this feature is to prevent the use of tokens with extreme long lifetimes.

Full changelog

Added the 'maximum_expiration' to the jwt plugin schema
Added a check in the handler
Added unit tests for handler and schema.

jeremyjpj0916 · 2018-03-26T05:32:08Z

I like the idea here, but not all JWT's will include the exp and nbf, why not also make an if validating on if (exp - ngx.time()) or w/e you have if you want to keep token expiry an hour or less? It's something we implemented too to prevent clients from being irresponsible with their tokens.

mvanholsteijn · 2018-03-26T08:41:19Z

I thought it would be cheaper to do the (exp -nbf) calculation. In addition, with (exp-nbf) I did not have to deal with clock skew.

I will change it, to use exp only. leaving the configuration of maximum_expiration to deal with possible clock skew..

Cheers,

Mark

mvanholsteijn · 2018-03-27T19:46:48Z

@jeremyjpj0916 can you please review my pull request? Once it is integrated, I can stop merging the changes from master into this one 😃

jeremyjpj0916 · 2018-03-28T06:46:42Z

@mvanholsteijn I don't have write access, I chimed in only because I was implementing a similar change personally and thought to bring up the scenario where just exp is exposed and to compare against ngx.time() , which is looks like you added that so overall I think this is a nice feature! Hopefully kong team agrees and will consider implementing it as it makes sense from a security perspective.

mvanholsteijn · 2018-03-28T07:16:28Z

@jeremyjpj0916 I did not know you did not have write access :-)

We implemented this feature after an external security audit was performed on our API. They recommended to limit the lifetime of the jwt token.

mvanholsteijn · 2018-05-07T07:33:25Z

@thibaultcha Do you have any idea on when this PR might be reviewed?

thibaultcha

@mvanholsteijn Thank you for the patch! And sorry for the delay.

Will you apply the recommended changes? Also, since this patch contains migrations, it must be opened against next, and not master. Please do not close this PR and open a new one, instead, use the GitHub UI to change the base of your PR (it might need rebasing as well).
Since the next release version is 0.14.0 anyway, there it should not delay this from being released.

thibaultcha · 2018-05-19T00:38:59Z

spec/03-plugins/17-jwt/03-access_spec.lua

+        local payload = {
+	  iss = jwt_secret.key,
+	  exp = os.time() + 3600,
+	  nbf = os.time() - 30


style: indentation is off here and in several other places in this file

thibaultcha · 2018-05-19T00:39:22Z

kong/plugins/jwt/schema.lua

+	end
+      end
+      if not has_exp then
+	 return false, Errors.schema "when you specify maximum_expiration, 'exp' must be in claims_to_verify"


style: indentation is off here and above in the same function

Trying to come up with a more standard/consistent error message:

"claims_to_verify must contain 'exp' when specifying maximum_expiration"

changed to "To set maximum_expiration, you need to add 'exp' in claims_to_verify"

thibaultcha · 2018-05-19T00:39:44Z

spec/03-plugins/17-jwt/03-access_spec.lua

+        path = "/request/?jwt=" .. jwt,
+        headers = {
+          ["Host"] = "jwt11.com"
+         }


ditto: indentation

thibaultcha · 2018-05-19T00:42:15Z

kong/plugins/jwt/schema.lua

@@ -18,5 +26,20 @@ return {
    claims_to_verify = {type = "array", enum = {"exp", "nbf"}},
    anonymous = {type = "string", default = "", func = check_user},
    run_on_preflight = {type = "boolean", default = true},
+    maximum_expiration = {type = "number", default = -1, func = check_maximum_expiration_positive},


I think a more common pattern is to treat 0 as the "indefinite expiration", and any value below zero as invalid (as if it were an unsigned integer). The semantics are somewhat more obvious (-1 has a strong "error-related" connotation and also feels strange in a time-related value).

you are right. fixed.

thibaultcha · 2018-05-19T00:45:37Z

kong/plugins/jwt/handler.lua

@@ -165,6 +165,15 @@ local function do_authentication(conf)
    return false, {status = 401, message = errors}
  end

+  -- Verify the JWT registered claims
+  if conf.maximum_expiration ~= nil and conf.maximum_expiration > 0 then


The second check of this condition seems unnecessary, since you included a safeguard to check_maximum_expiration() already (if max_exp <= 0)

I wanted to avoid calling the function as I know it is not necessary saving a few CPU cycles.

thibaultcha · 2018-05-19T00:49:06Z

kong/plugins/jwt/jwt_parser.lua

+    end
+  else
+      return false, {exp = "is missing"}
+  end


What do you think about: "exceeds maximum allowed expiration"?

Also, as per our contributing guide, we recommend hoisting short branches with early exits for clarity, and a few other minor nitpicks:

local exp = self.claims.exp if not exp then return false, { exp = "missing" } end if exp - ngx_time() > maximum_expiration then return false, { exp = "exceeds maximum allowed expiration" } end return true

I changed it to the message "exceeds maximum allowed expiration" if the 'exp' is missing.

thibaultcha · 2018-05-19T00:50:35Z

kong/plugins/jwt/schema.lua

@@ -8,6 +9,13 @@ local function check_user(anonymous)
  return false, "the anonymous user must be empty or a valid uuid"
 end

+local function check_maximum_expiration_positive(v)
+  if v < -1 or v == 0 then
+    return false, "maximum_expiration should be -1 or greater than 0"


No need to repeat the name of the property in this error message, it will already be contained in the key for this error. We are also lacking a test for this validation.

thibaultcha · 2018-05-19T00:52:03Z

kong/plugins/jwt/schema.lua

+      local has_exp = false
+      if plugin_t.claims_to_verify then
+	for index, value in ipairs(plugin_t.claims_to_verify) do
+	  has_exp = has_exp or value == "exp"


A more convenient pattern is to break out of the loop:

if value == "exp" then has_exp = true break end

mvanholsteijn · 2018-05-20T08:33:38Z

@thibaultcha I processed all (but 1) remarks.

thibaultcha

Hey @mvanholsteijn

I think this is almost ready. I was about to merge your patch manually with the modifications I just mentioned, but since there is a merge commit in your PR, it made things more difficult and I'd rather ask you to either properly rebase your branch on top of next, or made those modifications yourself (in which case I will squash + merge your changes). A proper rebase would be much appreciated as it makes our life a lot easier. For reference see the CONTRIBUTING.md guidelines which recommend following this process when contributing to this repo.

Thank you!

thibaultcha · 2018-05-21T23:35:23Z

kong/plugins/jwt/schema.lua

+       end
+      end
+      if not has_exp then
+        return false, Errors.schema "To set maximum_expiration, you need to add 'exp' in claims_to_verify"


A convention followed by absolutely all error messages in Kong is to not capitalize the sentences. Better not deviate from it (hence my previous suggestion).

thibaultcha · 2018-05-21T23:36:48Z

kong/plugins/jwt/schema.lua

@@ -8,6 +9,13 @@ local function check_user(anonymous)
  return false, "the anonymous user must be empty or a valid uuid"
 end

+local function check_positive(v)
+  if v < 0 then
+    return false, "should be >= 0"


Another convention our error messages follow is to use words instead of signs. In this case:

should be 0 or greater or: cannot be negative

thibaultcha · 2018-05-21T23:37:40Z

kong/plugins/jwt/jwt_parser.lua

+
+  local exp = self.claims["exp"]
+  if exp == nil or (exp - ngx_time()) > maximum_expiration then
+      return false, {exp = "exceeds maximum allowed expiration"}


style: the indentation in this codebase is 2 spaces (this one uses 4 and the above branch uses 3)

thibaultcha · 2018-05-21T23:38:01Z

kong/plugins/jwt/jwt_parser.lua

+-- @return error if any
+function _M:check_maximum_expiration(maximum_expiration)
+  if maximum_expiration <= 0 then
+     return true, nil


return true would be enough here as per our return conventions.

thibaultcha · 2018-05-21T23:39:32Z

kong/plugins/jwt/schema.lua

+    if plugin_t.maximum_expiration ~= nil and plugin_t.maximum_expiration > 0 then
+      local has_exp = false
+      if plugin_t.claims_to_verify then
+       for index, value in ipairs(plugin_t.claims_to_verify) do


style: indentation is weird here as well (1 space?)

mvanholsteijn · 2018-05-22T09:10:52Z

Fixed latest review comments and rebased on upstream/next.

PS: Is there a way to get luacheck to complain about indentation?

Support for limiting the expiration period on JWT tokens. In the JWT plugin you can set the property `maximum_expiration` to a positive integer, indicating the maximum number of seconds the `exp` claim in the token may be ahead in the future. From #3331 Signed-off-by: Thibault Charbonnier <[email protected]>

thibaultcha · 2018-05-23T01:45:52Z

Merged with some modifications (testing, formatting, errors), thanks again for your contribution

mvanholsteijn · 2018-05-23T10:06:29Z

@thibaultcha thank you for your support!

See Kong/kong#3331 From #707

mvanholsteijn changed the title ~~added support for maximum duration~~ added support for maximum expiration of a JWT token Mar 22, 2018

mvanholsteijn force-pushed the maximum_expiration branch from 76ef187 to 987cda5 Compare March 22, 2018 19:45

thibaultcha added the pr/please review label Mar 22, 2018

mvanholsteijn mentioned this pull request May 13, 2018

documented jwt parameter maximum_expiration for pr 3331 Kong/docs.konghq.com#707

Closed

thibaultcha requested changes May 19, 2018

View reviewed changes

thibaultcha added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/please review labels May 19, 2018

mvanholsteijn changed the base branch from master to next May 20, 2018 08:30

thibaultcha requested changes May 21, 2018

View reviewed changes

mvanholsteijn force-pushed the maximum_expiration branch from 44190ec to 20a8b3a Compare May 22, 2018 09:08

added maximum expiration for tokens on the jwt plugin

92a9b7c

mvanholsteijn force-pushed the maximum_expiration branch from 20a8b3a to 92a9b7c Compare May 22, 2018 10:55

thibaultcha closed this May 23, 2018

mvanholsteijn deleted the maximum_expiration branch May 23, 2018 10:06

thibaultcha pushed a commit to Kong/docs.konghq.com that referenced this pull request Jun 28, 2018

docs(jwt) add the 'maximum_expiration' configuration property

469e8ae

See Kong/kong#3331 From #707

ukiahsmith pushed a commit to Kong/docs.konghq.com that referenced this pull request Jul 2, 2018

docs(jwt) add the 'maximum_expiration' configuration property

c6621fb

See Kong/kong#3331 From #707

added support for maximum expiration of a JWT token #3331

added support for maximum expiration of a JWT token #3331

Conversation

mvanholsteijn commented Mar 22, 2018 • edited Loading

Summary

Full changelog

jeremyjpj0916 commented Mar 26, 2018 • edited Loading

mvanholsteijn commented Mar 26, 2018

mvanholsteijn commented Mar 27, 2018

jeremyjpj0916 commented Mar 28, 2018

mvanholsteijn commented Mar 28, 2018

mvanholsteijn commented May 7, 2018

thibaultcha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvanholsteijn May 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvanholsteijn commented May 20, 2018

thibaultcha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvanholsteijn commented May 22, 2018

thibaultcha commented May 23, 2018

mvanholsteijn commented May 23, 2018

mvanholsteijn commented Mar 22, 2018 •

edited

Loading

jeremyjpj0916 commented Mar 26, 2018 •

edited

Loading

mvanholsteijn May 20, 2018 •

edited

Loading